Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "enforceDefaultLimits" flag #4124

Merged
merged 3 commits into from
Jan 7, 2025
Merged

Conversation

greggman
Copy link
Contributor

I'm not sure what to do about this but dealing with dependent limits is kind of a PITA.

Tests that deal with storage buffers and storage textures need to take into account that they might have 0. But, they also need to be tested with the maxiumum number of storage buffers/textures.

So, if you set the test to use max storage buffers/textures then, unless you have a device that supports 0 you have no easy way to test that the test functions when the limit is 0.

While refactoring the tests I start without requesting the limit so on compat I get zero. Once that works I add MaxLimitsTestMixin or similar to request the maximum limits, otherwise a compat device would get no coverage.

But, now I have the issue that if I'm modifying the test I need to remember to test with 0 so I have to go manually comment out the code that's requesting max limits.

So, I thought I'd add this option.

Unfortunately, the default limits are defined in src/webgpu/capability_info.ts which is listed as off limits to navigator_gpu.ts. I tried moving the limits table to src/common/limits.ts but then src/webgpu/capabilities_info.ts complained that src/common/limits.ts is off limits. So, I'm not sure where I can put a shared limits.ts file that both navigator_gpu.ts and capabilities_info.ts can access.

In the webgpu-dev-extension, I implemented this by requesting 2 adapters and a temp device.

// pseudo code
tempAdapter = await requestAdapter(options);
tempDevice = await requestDevice(...)
const defaultLimits = tempDevice.limits;
tempDevice.destroy();
adapter = await requestAdapter(options)
adapter.limits = defaultLimits;

But I wasn't sure that was a good solution in the CTS vs using the limits table.

Also, in the webgpu-dev-extension I don't really care about actually enforcing the limits but in the cts, the limits tests in src/webgpu/api/validation/capability_checks/limits will fail if the limits are not enforced and I needed this flag to be able to debug them and make sure they work when a limit is 0.

Issue: #4120

@greggman greggman requested a review from kainino0x December 30, 2024 21:14
@kainino0x
Copy link
Collaborator

Unfortunately, the default limits are defined in src/webgpu/capability_info.ts which is listed as off limits to navigator_gpu.ts. I tried moving the limits table to src/common/limits.ts but then src/webgpu/capabilities_info.ts complained that src/common/limits.ts is off limits. So, I'm not sure where I can put a shared limits.ts file that both navigator_gpu.ts and capabilities_info.ts can access.

I think src/common/util/limits.ts would be allowed. I don't remember the rule syntax, but the message seems to imply that:

              {
                "target": "./src/webgpu",
                "from": "./src/common",
                "except": ["./framework", "./util"],
                "message": "Non-framework common/ code imported from webgpu/ suite"
              },

src/common/util/navigator_gpu.ts Outdated Show resolved Hide resolved
@greggman
Copy link
Contributor Author

greggman commented Jan 4, 2025

So I wrote this change before the update to the limits to add maxXXXInYYYStage. With those added this code broke because it would request limits that don't exist.

So, I made it so it makes a temp device. This might be fine but, tests that test low-power vs high-performance may have issues. I'd either have to always request a temp adapter and temp device OR, I'd have to turn the requestAdapter descriptor into a key to cache the limits. Can that wait or later or should I do something now?

Copy link
Collaborator

@kainino0x kainino0x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine, though I don't quite understand why it's needed. In requestAdapter, after having already called origRequestAdapterFn, we already have the list of supported limits.

src/common/runtime/helper/options.ts Outdated Show resolved Hide resolved
@greggman
Copy link
Contributor Author

greggman commented Jan 7, 2025

This is fine, though I don't quite understand why it's needed. In requestAdapter, after having already called origRequestAdapterFn, we already have the list of supported limits.

I'm not sure what you're saying is not needed. An actual adapter will always have max limits. We create a device with no requiredLimits to get the default limits. We then create an adapter and set all adapter.limits to the defaults so down stream it won't see the max limits. There are tests that check things throw if we request higher than max. These tests will fail to throw if we don't manually enforce that the max is the default (not the actual max). That's why we need the defaults.

@greggman greggman enabled auto-merge (squash) January 7, 2025 19:20
I'm not sure what to do about this but dealing with dependent
limits is kind of a PITA.

Tests that deal with storage buffers and storage textures need to
take into account that they might have 0. But, they also need to
be tested with the maxiumum number of storage buffers/textures.

So, if you set the test to use max storage buffers/textures then,
unless you have a device that supports 0 you have no easy way to
test that the test functions when the limit is 0.

While refactoring the tests I start without requesting the limit
so on compat I get zero. Once that works I add `MaxLimitsTestMixin`
or similar to request the maximum limits, otherwise a compat device
would get no coverage.

But, now I have the issue that if I'm modifying the test I need to
remember to test with 0 so I have to go manually comment out the code
that's requesting max limits.

So, I thought I'd add this option.

Unfortunately, the default limits are defined in src/webgpu/capability_info.ts
which is listed as off limits to navigator_gpu.ts. I tried moving the limits
table to src/common/limits.ts but then src/webgpu/capabilities_info.ts complained
that src/common/limits.ts is off limits. So, I'm not sure where I can put a
shared limits.ts file that both navigator_gpu.ts and capabilities_info.ts can access.

In the webgpu-dev-extension, I implemented this by requesting 2 adapters and a temp
device.

```js
// pseudo code
tempAdapter = await requestAdapter(options);
tempDevice = await requestDevice(...)
const defaultLimits = tempDevice.limits;
tempDevice.destroy();
adapter = await requestAdapter(options)
adapter.limits = defaultLimits;
```

But I wasn't sure that was a good solution in the CTS vs
using the limits table.

Also, in the webgpu-dev-extension I don't really care about
actually enforcing the limits but in the cts, the limits tests
in src/webgpu/api/validation/capability_checks/limits will fail
if the limits are not enforced and I needed this flag to be able
to debug them and make sure they work when a limit is 0.
The code already exits if a provider exists.
Originally I was using the limits in capability_info.ts
but passing unknown limits, like maxStorageBuffersInFragmentStage,
to an adapter that doesn't know that limit ends up causing an error.

So, instead, create temp adapter and from that a temp device
with no limits requested to query the limits. We can't query
until the first request since getGPU is not async.
@greggman greggman merged commit 63658e1 into gpuweb:main Jan 7, 2025
1 check passed
@greggman greggman deleted the enforce-default branch January 7, 2025 19:46
@kainino0x
Copy link
Collaborator

I'm not sure what you're saying is not needed. An actual adapter will always have max limits. We create a device with no requiredLimits to get the default limits. We then create an adapter and set all adapter.limits to the defaults so down stream it won't see the max limits. There are tests that check things throw if we request higher than max. These tests will fail to throw if we don't manually enforce that the max is the default (not the actual max). That's why we need the defaults.

Sorry, I mean I'm not sure why a temp device is needed. We know the default value for each limit from our info tables (as long as we know which featureLevel to use). From any adapter we can see which limits are supported by the browser (by whether they exist in adapter.limits). So in the shim for adapter.limits we can return only the limit names that the original adapter had, with the values reduced to the defaults.

Though I'm not sure if what I'm saying makes sense because I haven't tried it.

@greggman
Copy link
Contributor Author

greggman commented Jan 8, 2025

That makes sense. I can put up a new PR to use the limits table.

another thing we could do is just set all the max limits to 0 since the impl is supposed to set them to the default. For min limits though, some have restrictions like power-of-2 so I guess we'd still need the table.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants